Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide fillDescriptions for Pixel CPE classes #27694

Merged
merged 10 commits into from
Aug 28, 2019

Conversation

mmusich
Copy link
Contributor

@mmusich mmusich commented Aug 5, 2019

PR description:

This PR provides fillDescriptions methods for PixelCPE classes as requested in issue #27660.
The fillPSetDescription method is used when useful to recycle configuration from the helper classes.
As a result, all occurences of existsAs in RecoLocalTracker/SiPixelRecHits are removed.
The parameter DoCosmics (unused) is removed from the configuration files of PixelCPEClusterRepair and PixelCPETemplateReco though is maintained in the fillDescriptions as it happens to be in existing HLT configuration files.

PR validation:

It passes standard tests performed with addOnTests and

runTheMatrix.py -s -j 8 -t 4 --ibeos

if this PR is a backport please specify the original PR:

This PR is not a backport.
cc:
@tsusa @pmaksim1 @cmantill @OzAmram @tvami

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 5, 2019

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 5, 2019

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27694/11298

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 5, 2019

A new Pull Request was created by @mmusich (Marco Musich) for master.

It involves the following packages:

RecoLocalTracker/SiPixelRecHits

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@makortel, @felicepantaleo, @GiacomoSguazzoni, @rovere, @VinInn, @dkotlins, @gpetruc, @ebrondol, @threus this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@Dr15Jones
Copy link
Contributor

@mmusich thanks for doing the work!

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 5, 2019

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 5, 2019

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27694/11300

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 5, 2019

Pull request #27694 was updated. @perrotta, @cmsbuild, @slava77 can you please check and sign again.

@slava77
Copy link
Contributor

slava77 commented Aug 27, 2019

related to the cfi cleanup

I see that some explicit (re)definitions of the ES producers are present in the following (excluding auto-generated HLT files)

  • PixelCPEGenericESProducer
    • process.PixelCPEGenericESProducer = cms.ESProducer("PixelCPEGenericESProducer",
      EdgeClusterErrorX = cms.double(50.0),
      DoCosmics = cms.bool(False),
      LoadTemplatesFromDB = cms.bool(False),
      UseErrorsFromTemplates = cms.bool(False),
      eff_charge_cut_highX = cms.double(1.0),
      TruncatePixelCharge = cms.bool(False),
      size_cutY = cms.double(3.0),
      size_cutX = cms.double(3.0),
      inflate_all_errors_no_trk_angle = cms.bool(False),
      IrradiationBiasCorrection = cms.bool(False),
      TanLorentzAnglePerTesla = cms.double(0.106),
      inflate_errors = cms.bool(False),
      eff_charge_cut_lowX = cms.double(0.0),
      eff_charge_cut_highY = cms.double(1.0),
      ClusterProbComputationFlag = cms.int32(0),
      EdgeClusterErrorY = cms.double(85.0),
      ComponentName = cms.string('PixelCPEGeneric'),
      eff_charge_cut_lowY = cms.double(0.0),
      PixelErrorParametrization = cms.string('NOTcmsim'),
      Alpha2Order = cms.bool(True),
      Upgrade = cms.bool(True)
      )
    • hltESPPixelCPEGeneric = cms.ESProducer( "PixelCPEGenericESProducer",
      EdgeClusterErrorX = cms.double( 50.0 ),
      DoCosmics = cms.bool( False ),
      LoadTemplatesFromDB = cms.bool( True ),
      UseErrorsFromTemplates = cms.bool( True ),
      eff_charge_cut_highX = cms.double( 1.0 ),
      TruncatePixelCharge = cms.bool( True ),
      size_cutY = cms.double( 3.0 ),
      size_cutX = cms.double( 3.0 ),
      inflate_all_errors_no_trk_angle = cms.bool( False ),
      IrradiationBiasCorrection = cms.bool( False ),
      TanLorentzAnglePerTesla = cms.double( 0.106 ),
      inflate_errors = cms.bool( False ),
      eff_charge_cut_lowX = cms.double( 0.0 ),
      eff_charge_cut_highY = cms.double( 1.0 ),
      ClusterProbComputationFlag = cms.int32( 0 ),
      EdgeClusterErrorY = cms.double( 85.0 ),
      ComponentName = cms.string( "hltESPPixelCPEGeneric" ),
      eff_charge_cut_lowY = cms.double( 0.0 ),
      PixelErrorParametrization = cms.string( "NOTcmsim" ),
      Alpha2Order = cms.bool( True )
      )
  • PixelCPETemplateRecoESProducer
    • hltESPPixelCPETemplateReco = cms.ESProducer( "PixelCPETemplateRecoESProducer",
      DoCosmics = cms.bool( False ),
      LoadTemplatesFromDB = cms.bool( True ),
      ComponentName = cms.string( "hltESPPixelCPETemplateReco" ),
      Alpha2Order = cms.bool( True ),
      ClusterProbComputationFlag = cms.int32( 0 ),
      speed = cms.int32( -2 ),
      UseClusterSplitter = cms.bool( False )
      )

The overlapproblem_pixelphase1_cfg.py is apparently not used.
The SiStrip_OfflineMonitoring_cff defines a hltESPPixelCPEGeneric and hltESPPixelCPETemplateReco and is apparently included in the standard workflows via DQMOffline_cff.
I think that these other definitions should be eventually cleaned up and replaced by definitions from the reference cfis.

@slava77
Copy link
Contributor

slava77 commented Aug 27, 2019

+1

for #27694 6bf1f4c

  • code/configuration changes are in line with the PR description and the follow up review
  • jenkins tests pass and comparisons with the baseline show no differences

There are still a few places outside of the reference PixelCPE package definitions where the ES producer instance configurations can be cleaned up. This can be done in a follow up PR
#27694 (comment)

@Martin-Grunewald
Copy link
Contributor

+1

@jfernan2
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

@kpedro88
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 138dd8b into cms-sw:master Aug 28, 2019
@fwyzard
Copy link
Contributor

fwyzard commented Sep 12, 2019

I just stumbled upon this - it looks like RecoLocalTracker/SiPixelRecHits/python/PixelCPEFast_cfi.py was not updated to reflect these changes.

fwyzard added a commit to cms-patatrack/cmssw that referenced this pull request Sep 12, 2019
@makortel
Copy link
Contributor

@fwyzard PixelCPEFast exists only in the Patatrack fork.

@mmusich
Copy link
Contributor Author

mmusich commented Sep 12, 2019

I just stumbled upon this - it looks like RecoLocalTracker/SiPixelRecHits/python/PixelCPEFast_cfi.py was not updated to reflect these changes.

@fwyzard There is no such file in cmssw...

@fwyzard
Copy link
Contributor

fwyzard commented Sep 12, 2019

Yep, that's what I just realised... thanks.

makortel pushed a commit to makortel/cmssw that referenced this pull request Sep 13, 2019
fwyzard added a commit to cms-patatrack/cmssw that referenced this pull request Oct 8, 2020
fwyzard added a commit to cms-patatrack/cmssw that referenced this pull request Oct 19, 2020
fwyzard added a commit to cms-patatrack/cmssw that referenced this pull request Oct 20, 2020
fwyzard added a commit to cms-patatrack/cmssw that referenced this pull request Oct 23, 2020
fwyzard added a commit to cms-patatrack/cmssw that referenced this pull request Nov 6, 2020
fwyzard added a commit to cms-patatrack/cmssw that referenced this pull request Nov 16, 2020
fwyzard added a commit to cms-patatrack/cmssw that referenced this pull request Nov 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.